Repository navigation
feat(videoout): present reserved flips from tagged GPU completion - #5539
Conversation
|
Resolved the reported overlaps in published head Conventions and whitespace checks pass. The EOP validation/test registration was kept independent of the flip-master edits. The three native Radeon EOP cases pass on the matching combined integration. Fresh full Linux/Windows CI passes on Linux and Windows for this published head; expected skips remain unvalidated. The overlap workflow runs on main pushes and hourly, rather than PR pushes, so its earlier comment may remain until the next sweep. |
CnRJay
left a comment
CnRJay
left a comment
There was a problem hiding this comment.
Thorough tests. A few problems in the packet matching, which runs on every queue-0 submission of every title, not only on titles that call sceVideoOutSubmitEopFlip:
1. The match doesn't check the packet type or size before reading words[2] and words[7]. In holdFlipBehindWorker and reserveOutputs, the test is ((words[0] >> 8) & 0xff) == 0x49 && ((words[2] >> 24) & 7) == 2. A type-2 filler (header >> 30 == 2, one dword per PacketWords), or any non-type-3 header whose bits 15:8 happen to be 0x49, matches the opcode test. The code then reads up to 7 dwords past it, which can run past the end of the command buffer. Please require (words[0] >> 30) == 3 and cursor + 8 <= commands.size() (or PacketWords(words[0]) == 8) before looking at the body.
2. int_ctxid belongs to the guest, so the token range can collide with ordinary RELEASE_MEMs. Any RELEASE_MEM with INT_SEL == 2 whose context id falls in [EopFlipTokenBase, nextEopToken) and isn't a live reservation throws "EOP flip token is no longer reserved". Titles put their own values in int_ctxid, and nothing stops one from using 0x04xxxxxx. A title that never calls sceVideoOutSubmitEopFlip would only be affected after another caller had advanced nextEopToken, but the range check makes a guest value fatal. Tracking the token only where the guest stored it, or keying the match on tokens actually returned and not on the numeric range, would avoid failing on guest data the host never handed out.
3. The driver mutex is now held across the whole command scan. holdFlipBehindWorker used to scan without the lock and lock only when a flip was found. It now takes mutex before scanning, on every queue-0 submission from a guest thread, to read eopFlips. mutex is the driver's main lock, so this adds contention for every title. A cheap pre-scan without the lock (any type-3 RELEASE_MEM with INT_SEL == 2 at all), taking the lock only when one is found, keeps main's cost for everyone else.
4. Device lock. EOP-flip RELEASE_MEMs now skip deviceUse.lock() like the flip packet. Unlike the flip packet, they still go through preparePacketMemory, which can record the label write on the GPU (wroteOnGpu) through the device. Is that safe against device replacement without the shared deviceReplacement lock? If the flip part needs the lock released, the label part probably still needs it held.
Also, as far as I can tell, Unblocks: none and "this standalone change did not establish application startup" mean no title reaches this export on main yet. Per the recent export reviews, the maintainers may ask for that log line first.
|
Addressed all four packet concerns in The description still says Unblocks: none: the bundled-driver ABI probe is not evidence of a title stop on current main. No new title claim has been added. AI-assisted: yes. |
|
Checked the type/length/body checks before Non-blocking: removing the type-3/size check in |
Rebased onto main as one commit (conflicts in debt notes, Presentation.cpp includes and Submission.cpp helpers).
|
Rebased onto current main (conflicts in the debt notes, the include list of Non-blocking: removing the type-3/size check in |
09f73c4 to
8c0c449
Compare
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
|
With the queue-0 draw pipeline on, the default since boykopovar#1202, the packet loop hands every label-writing packet to the pipeline and skips the rest of the loop. A RELEASE_MEM that completes a reserved EOP flip (sceVideoOutSubmitEopFlip, boykopovar#5539) is a label write, so its label landed but the flip branch never ran: the reservation was consumed and the flip never presented. video_out_flip_eop and video_out_flip_eop_main failed and video_out_flip_eop_aftergpu timed out. Keep packets that carry a reserved flip out of the pipeline. They take the normal path, which drains the pipeline before the flip as it does for flip packets. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With the queue-0 draw pipeline on, the default since #1202, the packet loop hands every label-writing packet to the pipeline and skips the rest of the loop. A RELEASE_MEM that completes a reserved EOP flip (sceVideoOutSubmitEopFlip, #5539) is a label write, so its label landed but the flip branch never ran: the reservation was consumed and the flip never presented. video_out_flip_eop and video_out_flip_eop_main failed and video_out_flip_eop_aftergpu timed out. Keep packets that carry a reserved flip out of the pipeline. They take the normal path, which drains the pipeline before the flip as it does for flip packets. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 participants
What
Reserve an EOP flip and return an opaque DWORD token; a matching live reservation in a graphics-queue RELEASE_MEM packet drives presentation. Preserve label writes and normal queue events. Closing cancels unsubmitted reservations; tokens are unique across port generations.
The packet matcher now requires type 3, exactly eight words and a complete body before reading INT_SEL or INT_CTXID. Ordinary context IDs, including consumed/cancelled token values, remain ordinary label writes. A pre-scan avoids taking the driver mutex when no flip candidate is present. EOP label preparation/execution holds the device-replacement shared lock, then releases it before presentation as required by the existing flip path. Repeated live tokens within one submission are still rejected.
Also handle the trace-user-data WRITE_DATA emitted before this flip by updating UCONFIG state. Thread-trace capture and async-queue EOP flips remain unsupported.
The five-argument ABI and DWORD token use are inferred from Astro Bot PPSA21564's bundled AGC driver and the earlier isolated original-routine ABI probe. No title stop on current main, standalone application startup or measured console token encoding/copy width/capacity/error policy is established. These limits remain documented in technical debt.
Tested
Standalone head
09f73c4af643a32834d340adbba2759657d6b0abincludes upstream maina5f493f90dca348a65561aed4bb325a629d1b668. The bundled Linux Release library/test build passes. Direct X11eop,eop_mainandeop_aftergpupass on both AMD Radeon RX 6650 XT/RADV Mesa 26.2.4 and lavapipe Mesa 26.2.4, without skips. Coverage includes a type-2 filler carrying RELEASE_MEM opcode bits, malformed lengths, live-token duplicates, ordinary label writes with consumed/cancelled context IDs, close/reopen generations, capacity/concurrent callers and GPU work before presentation.Restoring only the former numeric token-range refusal makes the updated regression fail with
EOP flip token is no longer reserved. Restoring the source byte for byte, rebuilding and rerunning passes. The default offscreen CTest invocation skips all three display cases; those skips are not validation.Current conventions and whitespace checks pass. Fresh full Linux/Windows CI passes for this published head; expected skips remain unvalidated. Earlier CI and original-routine probe results refer to their prior revisions; no current Windows, full-suite, game-performance or console validation is claimed.
Checklist
Verified current full CI: run 38505683103 passes topic
09f73c4af643a32834d340adbba2759657d6b0abmerged into basea5f493f90dca348a65561aed4bb325a629d1b668. Linux 1059 passed/49 expected skips; Windows 730 passed/400 expected skips. Skipped cases remain unvalidated. The tested source tree matches this published topic head.